Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Go 1.13 and removing support for --dep-manager=dep #1949

Merged
merged 4 commits into from
Oct 23, 2019
Merged

Go 1.13 and removing support for --dep-manager=dep #1949

merged 4 commits into from
Oct 23, 2019

Conversation

joelanford
Copy link
Member

Description of the change:
Updates operator-sdk to require go1.13

This is will be a breaking change, and will include changes that drop support for creating and managing dep-based projects.

Motivation for the change:
Go 1.13 was recently released with improvements to error handling and go modules. Other related projects are working on upgrading as well (e.g. controller-runtime)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2019
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 20, 2019
@chenrui333 chenrui333 mentioned this pull request Oct 10, 2019
Closed
@joelanford joelanford mentioned this pull request Oct 16, 2019
Closed
4 tasks
@joelanford joelanford changed the title [WIP] Bump to go 1.13 Bump to go 1.13 Oct 16, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2019
@joelanford joelanford changed the title Bump to go 1.13 Go 1.13 and removing support for --dep-manager=dep Oct 20, 2019
@joelanford joelanford mentioned this pull request Oct 21, 2019
Merged
jmrodri
jmrodri requested changes Oct 21, 2019
View reviewed changes
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall, I have one nit which is the comment in the code seems incorrect.

.travis.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
cmd/operator-sdk/main.go Outdated Show resolved Hide resolved
cmd/operator-sdk/new/cmd.go Outdated Show resolved Hide resolved
@joelanford joelanford mentioned this pull request Oct 21, 2019
Closed
camilamacedo86
camilamacedo86 reviewed Oct 21, 2019
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regards the code it shows fine 🥇
Shows that it is just missing the CHANGELOG with the break changes.

I testing this PR now to see if all is fine too.

camilamacedo86
camilamacedo86 approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It worked fine locally in my basic tests.
All shows fine. I just think that it is missing the entries in the CHANGELOG over the goo upgrade and the removal of support for --dep-manager=dep.

@camilamacedo86
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2019
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Oct 21, 2019
@joelanford
CHANGELOG.md: add lines for operator-framework#1949
5e7a4e4
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2019
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Oct 21, 2019
@joelanford
CHANGELOG.md: add lines for operator-framework#1949
5b371ed
estroz
estroz reviewed Oct 22, 2019
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

release.md should be updated as well.

Otherwise LGTM

hasbro17
hasbro17 reviewed Oct 22, 2019
View reviewed changes
doc/sdk-cli-reference.md Outdated
### Flags

* `--dep-manager` string - Dependency manager file type to print (choices: "dep", "modules")

### Example

With dependency manager `dep`:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the example for --dep-manager=dep.

hasbro17
hasbro17 approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Just need to remove the operator-sdk print-deps --dep-manager=dep example from the sdk-cli-reference.md guide.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2019
estroz
estroz approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2019
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Oct 22, 2019
@joelanford
CHANGELOG.md: add lines for operator-framework#1949
39d8fce
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2019
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2019
@hasbro17
Copy link
Contributor

/retest

@joelanford
Copy link
Member Author

/test e2e-aws-subcommand

@joelanford
Copy link
Member Author

/test e2e-aws-go
/test e2e-aws-helm

@joelanford
Copy link
Member Author

/test e2e-aws-helm

@joelanford joelanford merged commit 347aaa1 into operator-framework:master Oct 23, 2019
@joelanford joelanford deleted the go1.13 branch October 23, 2019 16:33
@joelanford joelanford mentioned this pull request Oct 23, 2019
Merged
@periklis periklis mentioned this pull request Feb 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz approved these changes

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@hasbro17 hasbro17 hasbro17 approved these changes

@jmrodri jmrodri jmrodri approved these changes

@fabianvf fabianvf Awaiting requested review from fabianvf

@jmccormick2001 jmccormick2001 Awaiting requested review from jmccormick2001

Assignees

@camilamacedo86 camilamacedo86

@estroz estroz

Labels
size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joelanford @camilamacedo86 @openshift-ci-robot @hasbro17 @jmrodri @estroz